-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(types/ref): allow getter and setter types to be unrelated #11442
Conversation
Size ReportBundles
Usages
|
This comment was marked as resolved.
This comment was marked as resolved.
Hello team, could you please help run the ecosystem-ci? Thanks! |
/ecosystem-ci run |
@jh-leong I've invited you to the team btw. Thank you for your contributions! |
📝 Ran ecosystem CI: Open
|
@johnsoncodehk |
@jh-leong @yyx990803
Shouldn't it be Consider this example as well:
|
Hi @mefcorvi, thanks for your feedback. The behavior in the examples you mentioned is expected, see TypeScript Playground. I'm not entirely sure what specific issue you're referring to. Could you please create a new issue and provide a concrete example that illustrates the problem? This would help us track and address the issue more effectively. Thanks. |
To be honest, I didn't expect that it was possible to put one I apologize for the confusion and any inconvenience this may have caused. My question is resolved. |
@jh-leong After some thought, I conclude that something is indeed broken. Now, it is possible to have code like this, which is correct from a types standpoint but obviously broken. Consider this example: const b = ref(ref(1));
console.log(b.value.toFixed()); // 1
b.value = ref(2);
console.log(b.value.toFixed()); // runtime error Here, the type of Additionally, I believe that before the change in the current PR, it was not possible to set the value of a ref to another ref because of both setters and getters were UnwrapRef. Looking at the implementation of the ref function, I don't think that having one ref inside another was ever an allowed case. I will create a separate issue since this PR is already merged. |
I noticed this PR was merged into main instead of minor. This may require TS > 5.1 in the next patch, so we should move it to minor. |
I think the |
close #6766